fix(finalizer): Handle 1:many withdrawal:calldata #953
Conversation
The previous change to the finalizer incorrectly assumed that there would be a 1:1 relationship between a txn and a withdrawal or proof. This is incorrect on Polygon, where 2 txns are needed to finalize a single withdrawal. This change seems like a manageable way of establishing a 1:many relationship between withdrawal/proof logging and the necessary transaction(s), without imposing any changes on the underlying chain adapters. Ultimately it would be nice to backport this into the adapters themselves, but that's invasive and it's not worth prioritising now.
pxrl
requested review from
mrice32,
nicholaspai and
james-a-morris
as code owners
|
nb. I am testing this locally and will attempt to front-run the production finalizer so I can capture the withdrawals and verify this works as I hope. |
src/finalizer/index.ts
Outdated
| finalizationsToBatch.push(...txns); | ||
| // Append calldata. If multiple calls are needed per withdrawal (i.e. Polygon), | ||
| // require that the 2nd batch is appended to the first. | ||
| txns.forEach((txn, i) => withdrawals[i % withdrawals.length].calldata.push(txn)); |
There was a problem hiding this comment.
what if a future network uses more than 2 calls?
There was a problem hiding this comment.
As long as they are returned as batches that are multiples of withdrawals, i.e. batch 1, batch 2, batch 3, in the same order, then it should be OK.
ATM it seems unlikely that we'd require more than 2 transactions for a single withdrawal, and the fact that we currently need 2 seems to be exclusively a quirk of Polygon. Ultimately I'd like to go back into each of the adapters so that they enforce the 1:many relationship between withdrawal and multicall transactions, but I don't think that's a priority at the moment.
FWIW, I've been front-running the production finalizer successfully for the past ~24 hours, and it's working as expected.
Multiple layers of naming make this a bit messy.
|
I've run this continuously over the weekend. Aside from a small redis issue (unrelated to this change), the change has worked 100% as intended). Will submit as soon as CI passes. |
The previous change to the finalizer incorrectly assumed that there would be a 1:1 relationship between a txn and a withdrawal or proof. This is incorrect on Polygon, where 2 txns are needed to finalize a single withdrawal.
This change seems like a manageable way of establishing a 1:many relationship between withdrawal/proof logging and the necessary transaction(s), without imposing any changes on the underlying chain adapters. Ultimately it would be nice to backport this into the adapters themselves, but that's invasive and it's not worth prioritising now.